-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(warnings): add build.warnings option #14388
base: master
Are you sure you want to change the base?
Conversation
☔ The latest upstream changes (presumably #14423) made this pull request unmergeable. Please resolve the merge conflicts. |
tests/testsuite/warning_override.rs
Outdated
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s | ||
|
||
"#]] | ||
}); | ||
|
||
const ALLOW_CACHED: LazyLock<Inline> = LazyLock::new(|| { | ||
str![[r#" | ||
[WARNING] unused variable: `x` | ||
... | ||
[WARNING] `foo` (bin "foo") generated 1 warning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this line omitted and should we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we decided that we wanted allow
to not print any warnings / warning summary.
tests/testsuite/warning_override.rs
Outdated
"#]] | ||
}); | ||
|
||
fn make_project(main_src: &str) -> Project { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'm always a bit cautious of sharing code too much in tests. In this case, I think it works as the intent of the tests isn't obscure. The main question is the make_project
call, it did take me a second when reading the code to think "yeah, let x = 3;
is being meant for main
. One option would be to move the warning into this and call it make_project_with_rustc_warning
or something.
Hmm, thats a good point. When we add the checking of cargo warnings, we'll need tests for those as well. The names are fairly general in this code and it will all need to be modified in the follow up. Clarifying things now can make it clearer for now and make it easier to introduce new tests in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to make_project_with_rustc_warning
Thanks for the updates! |
f364c80
to
6c80335
Compare
☔ The latest upstream changes (presumably #14137) made this pull request unmergeable. Please resolve the merge conflicts. |
What does this PR try to resolve?
Allows control over warnings via a new Cargo configuration option
build.warnings
.The primary use case is wanting CI runs to deny warnings, while keeping them as warnings for local development. While this is currently possible by conditionally setting
RUSTFLAGS
in CI, that approach is not available for projects that already use another mechanism for settingRUSTFLAGS
, since the two would conflict. Additionally, we may in the future make this apply to more than warnings fromrustc
.build.warnings
config option.Option values:
deny
emits an error if any warnings occurredwarn
is the current existing behaviorallow
hides the warningsThe option currently only applies to warnings coming from
rustc
, however it's documented as allowing additional categories of warnings from Cargo to be added.Fixes #8424
Fixes #14258
How should we test and review this PR?
Tests are included in the PR. For local testing, after building
cargo
with this PR, run